fix(link): align app-router prefetch scheduling with intent#1149
Conversation
commit: |
|
@james-elicx let me know if you like this style of PR body better. Alternatively I could also do a proper HTML + CSS visualisation for more complex PR |
Link currently schedules viewport RSC prefetches outside production and does not promote user mouse or touch intent into an immediate prefetch. That diverges from Next.js Link behavior and can waste dev-server work during rapid iteration while missing an important production navigation warmup path. The shim now gates automatic prefetching to production, shares same-origin prefetch normalization, and schedules high-priority RSC prefetches from mouse and touch intent while preserving user handlers. Focused Link navigation tests cover development suppression, intent prefetch, prefetch=false, and dangerous-link handler passthrough.
Link currently schedules viewport RSC prefetches outside production and does not promote user mouse or touch intent into an immediate prefetch. That diverges from Next.js Link behavior and can waste dev-server work during rapid iteration while missing an important production navigation warmup path. The shim now gates automatic prefetching to production, shares same-origin prefetch normalization, and schedules high-priority RSC prefetches from mouse and touch intent while preserving user handlers. Focused Link navigation tests cover development suppression, intent prefetch, prefetch=false, and dangerous-link handler passthrough.
f9f9dd2 to
d7e142b
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Good PR. Clean extraction of the prefetch decision logic into a testable module, correct Next.js parity for dev-mode suppression and intent prefetch, and honest cleanup of E2E tests that were asserting production-only behavior against the dev server.
A few things worth addressing:
Design notes
-
Redundant decision calls: Both
viewportPrefetchDecisionandintentPrefetchDecisioncallgetLinkPrefetchDecisionwith identical inputs exceptintent. Since the current decision function returnsshouldPrefetch: falsefor the same conditions regardless of intent (the intent only affects priority), you're computing the same rejection logic twice per render. Today these are cheap, but you could compute the decision once and derive priority from intent when needed. Not blocking — just noting the redundancy. -
prefetch={true}vsprefetch={null}(default): Next.js App Router distinguishes these —prefetch={true}does a full prefetch including dynamic data, while the default (null) only prefetches the loading boundary (partial prefetch). The currentgetLinkPrefetchDecisiontreatstrue,null, andundefinedidentically. The PR body correctly notes partial prefetch as a non-goal, which is fine. Just confirming this is intentional and the discriminated union type won't mislead future contributors into thinking these cases are handled differently. -
Removed interception-context prefetch E2E test: The deleted test in
advanced.spec.tswas the only E2E test verifying that prefetches from different interception contexts (feed vs gallery) produce separate cache entries. The unit-level coverage intests/prefetch-cache.test.tsstill covers this at the cache layer, but there's no longer an integration-level test exercising the full flow (viewport observe → prefetch → cache key separation). Worth filing a follow-up to restore this test once production-mode E2E is feasible, so the interception-context cache-key separation stays protected at the integration level.
Minor issues
See inline comments.
| const shouldViewportPrefetch = viewportPrefetchDecision.shouldPrefetch; | ||
| const viewportPrefetchPriority = viewportPrefetchDecision.shouldPrefetch | ||
| ? viewportPrefetchDecision.priority | ||
| : "low"; |
There was a problem hiding this comment.
These fallback values ("low" / "high") are dead code — they're only read when shouldPrefetch is false, and in that case neither prefetchUrl nor the observer callback is ever invoked. They exist solely to avoid a conditional access on the discriminated union, but they introduce a false signal that priority matters in the disabled case.
Consider making the intent explicit:
| : "low"; | |
| const shouldViewportPrefetch = viewportPrefetchDecision.shouldPrefetch; | |
| const viewportPrefetchPriority = viewportPrefetchDecision.shouldPrefetch | |
| ? viewportPrefetchDecision.priority | |
| : undefined; | |
| const shouldIntentPrefetch = intentPrefetchDecision.shouldPrefetch; | |
| const intentPrefetchPriority = intentPrefetchDecision.shouldPrefetch | |
| ? intentPrefetchDecision.priority | |
| : undefined; |
Then the call sites would use viewportPrefetchPriority! (or assert) only inside the shouldViewportPrefetch guard, making it clear priority is never used when prefetch is disabled. Not blocking, but reduces the misleading defaults.
| currentOrigin: string | undefined; | ||
| }): string | null { | ||
| const { href, basePath, currentOrigin } = input; | ||
| if (!isAbsoluteOrProtocolRelative(href)) return href; |
There was a problem hiding this comment.
Nit: for a non-absolute, non-protocol-relative href that doesn't start with / (e.g. a bare relative path like "foo" or "./bar"), this returns the raw string. That's fine because prefetchUrl and the viewport observer both run it through toBrowserNavigationHref downstream which resolves against window.location.href, but the contract of this function isn't obvious from its signature alone — it can return relative paths that aren't valid prefetch targets on their own.
A doc comment noting that callers must resolve relative results before use would help.
| } | ||
| return <a {...anchorProps}>{children}</a>; | ||
| return ( | ||
| <a {...anchorProps} onMouseEnter={onMouseEnter} onTouchStart={onTouchStart}> |
There was a problem hiding this comment.
Good catch preserving user handlers on dangerous links. One thing: the non-dangerous path uses handleMouseEnter / handleTouchStart (which wrap user handlers + prefetchOnIntent), but the dangerous path passes the raw onMouseEnter / onTouchStart directly. This is correct because prefetchOnIntent would bail out for dangerous hrefs anyway (via the decision function), but the asymmetry makes the two branches look different when they'd behave the same. Using handleMouseEnter / handleTouchStart here too would unify the branches and make it self-evident that prefetch is suppressed via the decision function, not via code path divergence.
Not blocking since behavior is correct either way.
| // This is a tactical escape hatch for Link only. It intercepts React and JSX | ||
| // runtime output because the current E2E setup cannot honestly reach the | ||
| // production-only Link prefetch path. Do not reuse it as a component harness. | ||
| function mockReactAnchorCaptureForLinkOnly_DO_NOT_REUSE( |
There was a problem hiding this comment.
Appreciate the loud name. The _DO_NOT_REUSE suffix is good, but consider also adding a brief note explaining why this approach is fragile: it mocks useEffect synchronously (skipping React's commit-phase scheduling) and intercepts createElement/JSX at the call site (not at the reconciler level), so it can't test re-renders, cleanup, or conditional effect execution. This context would help future contributors understand why they shouldn't copy this pattern for other component tests.
| if (key.includes("no-prefetch.rsc")) return true; | ||
| } | ||
| return false; | ||
| await page.evaluate(async () => { |
There was a problem hiding this comment.
This manual fetch call is a clever way to verify the instrumentation is working before asserting on the absence of prefetch fetches. Worth a brief inline comment explaining the intent:
| await page.evaluate(async () => { | |
| // Verify the fetch instrumentation is capturing .rsc URLs correctly | |
| // before relying on it to assert the absence of prefetch fetches. | |
| await page.evaluate(async () => { | |
| await window.fetch("/nextjs-compat/prefetch-test/target.rsc"); | |
| }); |
| }); | ||
| } | ||
|
|
||
| async function flushPrefetchTasks(): Promise<void> { |
There was a problem hiding this comment.
Three microtask ticks may not be sufficient if the prefetch path gains an additional async hop (e.g., createRscRequestUrl is async). Consider adding a brief comment explaining why three ticks is currently sufficient:
| async function flushPrefetchTasks(): Promise<void> { | |
| async function flushPrefetchTasks(): Promise<void> { | |
| // prefetchUrl schedules via requestIdleCallback (mocked as sync) then | |
| // enters an async IIFE with a single await (createRscRequestUrl). Three | |
| // microtask ticks drain the current chain. Update if the async depth grows. | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| } |
|
Review posted. Here's a summary of my findings: Overall: Solid PR. The extraction of prefetch decision logic into Design-level notes:
Inline comments:
|
|
/bigbonk review |
There was a problem hiding this comment.
Good PR. The extraction of prefetch eligibility into link-prefetch.ts, the production-only gate, and the intent prefetch handlers are all correct and well-aligned with Next.js behavior.
What I verified
canLinkPrefetchcorrectly gates onproduction+prefetch !== false+!isDangerous. This matches the Next.js viewport prefetch guard inpackages/next/src/client/components/links.ts.getLinkPrefetchHrefproperly handles same-origin, cross-origin, protocol-relative, and basePath cases. Thetry/catcharound URL parsing is the right defensive choice.- The dangerous-link branch already uses
handleMouseEnter/handleTouchStart(not raw handlers), so user handlers are preserved and prefetch correctly no-ops viashouldPrefetch === false. This is unified with the normal path. prefetchOnIntentcorrectly bails viashouldPrefetchbefore callingprefetchUrl, so dangerous hrefs,prefetch={false}, and dev mode all skip the fetch without needing separate code paths.- The viewport observer effect correctly depends on
[shouldPrefetch, localizedHref]and callsgetLinkPrefetchHrefagain for the observer callback, which is necessary because the effect captures the href at registration time. - The E2E test cleanup is honest — the removed tests were asserting production-only viewport prefetch behavior against the dev server, which was never valid.
getLinkPrefetchDecisionis exported and tested but unused in production code. This is fine as a public API for the module, but see minor note below.
Notes for the previous review
The previous automated review contained several inaccurate observations that I want to correct:
- The comment about "dead-code fallback priorities" and
viewportPrefetchDecision/intentPrefetchDecisionvariables doesn't match this code. The PR usescanLinkPrefetch(boolean), notgetLinkPrefetchDecision(discriminated union), in the component. - The comment about the dangerous path "passing raw
onMouseEnter/onTouchStartdirectly" is wrong — the dangerous branch already useshandleMouseEnter/handleTouchStart.
Minor notes (non-blocking)
See inline comments.
| shouldPrefetch: true, | ||
| priority: input.intent === "intent" ? "high" : "low", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Nit: getLinkPrefetchDecision is exported and tested but not used by any production code — only canLinkPrefetch and getLinkPrefetchHref are imported in link.tsx. If this is intentional as a public API for consumers or future use, that's fine, but a brief comment noting its purpose would help avoid someone removing it as dead code during cleanup.
| } | ||
| return <a {...anchorProps}>{children}</a>; | ||
| return ( | ||
| <a {...anchorProps} onMouseEnter={handleMouseEnter} onTouchStart={handleTouchStart}> |
There was a problem hiding this comment.
Good: unlike the previous review's claim, the dangerous branch correctly uses handleMouseEnter/handleTouchStart (the wrapped handlers), not the raw props. This means both branches go through the same handler path, and prefetch is suppressed via shouldPrefetch === false in prefetchOnIntent. Clean.
| // production-only Link prefetch path. It mocks useEffect synchronously and | ||
| // captures element creation before reconciliation, so it cannot test commit | ||
| // scheduling, cleanup, re-renders, or conditional effect execution. Do not | ||
| // reuse it as a component harness. |
There was a problem hiding this comment.
The _DO_NOT_REUSE suffix and the preamble comment are good guardrails. One addition that would help: note that the mocked useEffect executes synchronously during render (no commit phase), so effect cleanup functions are never called. This matters if someone later adds a test that depends on cleanup behavior.
| return false; | ||
| // Verify the fetch instrumentation sees .rsc URLs before relying on it | ||
| // to assert that Link prefetch does not issue a no-prefetch request. | ||
| await page.evaluate(async () => { |
There was a problem hiding this comment.
Good technique — verifying the instrumentation captures .rsc URLs before relying on it for absence assertions. This prevents false negatives where the test passes because the monkey-patched fetch wasn't set up correctly.
| @@ -286,23 +270,6 @@ test.describe("Intercepting Routes", () => { | |||
| await expect(page.locator('[data-testid="feed-page"]')).toBeVisible(); | |||
There was a problem hiding this comment.
The removed test was the only E2E-level test for interception-context cache-key separation (feed vs gallery producing separate prefetch cache entries). The unit test in tests/prefetch-cache.test.ts (line 174) still covers the cache-key logic directly, so the behavior is still protected. Worth a follow-up to restore this at the integration level once production-mode E2E testing is feasible.
|
Review submitted with approval. Here's a summary: Approved — the PR is correct and well-structured. Key findings:
Non-blocking notes were left inline about documenting |
Review dashboard
<Link>prefetch behavior for dev mode and user intent.packages/vinext/src/shims/link.tsx, thentests/link-navigation.test.ts, then the E2E expectation cleanup.Why
App Router
<Link>prefetching should follow the same basic expectation as Next.js: automatic prefetch is a production behavior, and strong navigation intent can warm the next RSC payload before click. This PR restores that expectation in the Link shim without changing router navigation or RSC cache semantics.onMouseEnterandonTouchStartnow schedule high-priority RSC prefetch after user handlers run.getPrefetchHref()before constructing the RSC prefetch URL.What changed
IntersectionObserverand prefetched with low fetch priority.prefetch={false}javascript:Maintainer review path
packages/vinext/src/shims/link.tsxshouldPrefetch,getPrefetchHref(),prefetchUrl(),prefetchOnIntent(), and rendered anchor handlers. The important behavior is production gating, shared same-origin normalization, low versus high fetch priority,prefetch={false}, and dangerous href handler preservation.tests/link-navigation.test.tstests/e2e/app-router/nextjs-compat/prefetch.spec.tsprefetch={false}target is not cached in dev.tests/e2e/app-router/advanced.spec.tsTests
Added or changed coverage
tests/link-navigation.test.tstests/link-navigation.test.tstests/link-navigation.test.tstests/link-navigation.test.tstests/link-navigation.test.tsprefetch={false}disables intent and viewport prefetch.tests/link-navigation.test.tstests/e2e/app-router/nextjs-compat/prefetch.spec.tsprefetch={false}target is not cached.tests/e2e/app-router/advanced.spec.tsCommands actually run
vp test run tests/link-navigation.test.ts tests/link.test.ts tests/prefetch-cache.test.ts tests/nextjs-compat/prefetch.test.tsPLAYWRIGHT_PROJECT=app-router vp run test:e2e tests/e2e/app-router/nextjs-compat/prefetch.spec.ts tests/e2e/app-router/advanced.spec.tsvp checkpackages/vinext/src/server/request-pipeline.ts:604.Risk / compatibility
prefetch={false}, user event handlers, and dangerous href behavior remain supported.Non-goals
prefetch={null}/auto.rscpayloads. Matching Next.js partial prefetch requires router/server support for loading-boundary or segment-level prefetch responses, not just a Link shim change.References